Migrate useSingleExecution off InteractionManager#95391
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
posted a discussion on an alternate solution: https://expensify.slack.com/archives/C01GTK53T8Q/p1783451929635129 |
|
@Pujan92 @ikevin127 One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 781df22a2e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
…when it has UX regression because of debounce
roryabraham
left a comment
There was a problem hiding this comment.
Let's step back ... it seems like we are headed towards a more robust solution in this thread, without any delay/throttling issues, or needing to manually track the buttons for which a double-tap might trigger a double-navigation.
Meanwhile, the current solution using the (albeit deprecated) InteractionManager doesn't have these pitfalls either.
So I suggest that we close this pull request, and instead pursue the better solution described here, keeping the deprecated InteractionManager-based solution in the meantime.
|
@roryabraham Yeah, I agree with you, it’s not a clean migration. I’ve tried to add Since we are currently on version I think it might be a good idea to spin up a quick test app first, just to make sure this approach works for us. If it looks good, we can go ahead with the update. I have some availability right now, so I’d be happy to take this on and start the update if we want to go that route 🙌 |
|
sounds good |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0dd7fb8e4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
war-in
left a comment
There was a problem hiding this comment.
Changes lgtm, left a few comments
| COMPOSER_FOCUS_DELAY: 150, | ||
| MAX_TRANSITION_DURATION_MS: 1000, | ||
| MAX_TRANSITION_START_WAIT_MS: 1000, | ||
| NAVIGATION_PREDICTION_WINDOW_MS: 150, |
There was a problem hiding this comment.
I'd verify if 150ms is enough even on low-end android devices
There was a problem hiding this comment.
I’ve investigated runAfterPredictedTransition on a lower-end device (Motorola Moto G15), and the results are pretty interesting.
The registered 150ms safety timeout is blocked and does not run while the js thread is busy with react’s state update and commit. After react commits, that overdue timeout races the state listener: react navigation emits the new state from useEffect (after commit), so once the thread is free again the timeout can fire either before or after the listener sees the update.
I fixed this by adding an extra check when the 150ms timeout runs: before clearing the prediction, it checks whether the navigation ref already has the new focused route. That check is reliable for normal navigations, because the ref store is updated during react navigation setState, before commit, so the new route is already visible even if the state listener has not run yet.
| // never move the focused route. JUMP_TO can, but only tab navigators use it and they do not emit | ||
| // transitionStart/transitionEnd, so waiting on TransitionTracker would never settle. |
There was a problem hiding this comment.
I wonder if we should try to support JUMP_TO in TransitionTracker 🤔 From what the comment says, it's not an issue at the moment, but we could be future-proof and handle it right away
Of course, we should do it in a separate issue
Let me know if that's worth investigating @roryabraham @collectioneur
There was a problem hiding this comment.
I’ve investigated this for a bit and confirmed that it’s possible to track tab transitions. Although we don’t need to remove JUMP_TO from here since startTransition for tabs will be registered synchronously with dispatch. I’ve opened a draft so we can add this once this PR gets merged 🙂
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0dd7fb8e4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
JakubKorytko
left a comment
There was a problem hiding this comment.
nice direction overall! most of my comments are around scoping tab-level tracking, a couple edge cases in the prediction helper, and test coverage gaps
| InteractionManager.runAfterInteractions(() => { | ||
| // Re-enables the button once the predicted (or actual) transition triggered by this press | ||
| // ends - or immediately, if the press wasn't predicted to cause one. | ||
| transitionHandleRef.current = runAfterPredictedTransition(() => { |
There was a problem hiding this comment.
unmount cleanup cancels the transition handle, but if action() returned a Promise, execution.finally inside this callback can still call setIsExecuting(false) afterward. That is the same class of issue as the old uncancelled debounce timer. A small mounted/ref guard around the setIsExecuting(false) paths would prevent a post-unmount state update when the async action settles late
There was a problem hiding this comment.
I looked into this but I don't think a mounted guard buys us anything here. We're on React 19, where calling a state setter on an unmounted component is a no-op
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 274acb8e8b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Thanks, I will check the thread discussion and pr tomorrow |
Explanation of Change
Migrates native
useSingleExecutionoffInteractionManager.runAfterInteractionsonto a new heuristic helper,runAfterPredictedTransition, and fixes twoTransitionTracker/ScreenLayoutbugs that made that wait unreliable.Bug fixes in
transitionStart/transitionEndtrackingTab transitions were unreliable on Android for every tab, and on iOS for the Home tab specifically. Besides Home, the other bottom-tab routes host nested split navigators whose screens register via stack
screenLayout, but that registration relies ontransitionStart/transitionEnd, whichreact-native-screensonly fires when tied to an actual view animation. Since tab switches use animation: 'none', no such animation ever runs on Android, so the nested-stack signal never fires there either, it just happened to look fine on iOS, whereviewWillAppearfires on any visibility change regardless of animation. Home was simply the most visible case on any platform, since it has no nested navigator and thus noscreenLayoutwiring at all.Fixed by wiring
bottomTabScreenLayoutWrapperdirectly intoTabNavigator's ownscreenLayout, so tracking now comes from bottom-tabs' owntransitionStart/transitionEnd, which fires consistently on both platforms regardless of the animation option.Unmount can leave a stuck open transition. A screen may emit
transitionStartand then unmount beforetransitionEnd; cleanup unsubscribes the listeners, sotransitionEndnever arrives and queued callbacks wait up toMAX_TRANSITION_DURATION_MS(1s). Fixed by callingTransitionTracker.endTransitionon unmount whenever a handle is still open.runAfterPredictedTransition(prediction mechanism)Naive
TransitionTracker.runAfterTransitions()right after a press usually sees zero active transitions (registration gap) and unlocks too early. Always usingwaitForUpcomingTransition: truewould make non-navigating presses wait up to 1s. The new helper predicts whether the current press will cause a visual transition by watchingnavigationRef__unsafe_action__+state: if a navigable action is followed by a focused-route change withinNAVIGATION_PREDICTION_WINDOW_MS(150ms), it waits for that upcoming transition to finish; otherwise it settles immediately.useSingleExecutionthen re-enables the button after the predicted (or already-active) transition ends - or right away when no transition is predicted.Fixed Issues
$ #71913
$ #83071
Tests
Tests (on iOS and Android):
Offline tests
N/A
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2026-07-07.at.12.41.22.mov
Android: mWeb Chrome
iOS: Native
Screen.Recording.2026-07-07.at.12.46.48.mov
iOS: mWeb Safari
MacOS: Chrome / Safari